style: address flake8 e402 warning in the codebase #3354
Conversation
github-actions
bot
added
GUI
python/grass/docs/conf.py
Outdated
| if not os.getenv("GISBASE"): | ||
| sys.exit("GISBASE not defined") |
There was a problem hiding this comment.
This change of moving the check under has the sys.path.insert calls won't have the expected outcome. Starting at line 24, that environment variable is used. Having the check after having finished using it isn't equivalent.
There was a problem hiding this comment.
Thank you for pointing that out. I've reconsidered the placement of the check and modified the code to ensure its effectiveness. I will push the changes to update the PR.
|
|
||
| def import_grass_libs(): | ||
| import grass.lib.gis as libgis | ||
| libgis.G_gisinit("") | ||
| import grass.lib.raster as libraster | ||
| import grass.lib.rowio as librowio | ||
| return libgis, libraster, librowio | ||
|
|
||
|
|
||
| libgis, libraster, librowio = import_grass_libs() |
There was a problem hiding this comment.
Using that pattern might complicate static analysis, as the imports aren't directly visible (it is a kind of lazy loading, but the call to that function is made during a parsing of the file for an import for example).
Does the lib raster and lib rowio import successfully and work correctly if libgis.G_gisinit("") isn't called strictly before, but underneath?
This pattern seems to be on multiple files.
There was a problem hiding this comment.
Thank you for highlighting the concern. I agree, and to address it, I've modified the code on all the changed files to keep the imports at the module level for direct visibility while maintaining the lazy loading pattern. I will update the PR with these changes
Sharansrj567
force-pushed
the
fix-flask8-e402-warning
branch
from
0ea796a to
ea17773
Compare
gui/wxpython/gmodeler/frame.py
Outdated
There was a problem hiding this comment.
This file is most probably fine.
| 0, os.path.abspath(os.path.join(os.environ["GISBASE"], "etc", "python", "grass")) | ||
| ) | ||
|
|
||
| from grass.script import core | ||
|
|
There was a problem hiding this comment.
This might not behave successfully. Is there a way to know if it imports well, and not only wrapped inside a grass --exec python. I've hit some long import chains that needed everything already up and running in order to have something similar working. To confirm.
There was a problem hiding this comment.
I ran GRASS GIS, accessed the interactive terminal, entered the Python shell within that environment, and executed the import statement without encountering any errors.
There was a problem hiding this comment.
In fact, I see that this file is for generating docs (sphinx). Does someone know how to trigger it to check it? I know that the docs are built daily on an OSGeo server
gui/wxpython/datacatalog/tree.py
Outdated
gui/wxpython/core/watchdog.py
Outdated
| from core.debug import Debug | ||
| from core.globalvar import SCT_EXT | ||
|
|
||
| from grass.script import core as grass |
There was a problem hiding this comment.
Even if that's not from this PR, this doesn't sound right, and quite ambiguous (the alias's name)
There was a problem hiding this comment.
I've noticed that across our codebase, there is a consistent use of the alias grass for the core module. Given that this pattern is present in around 51 files, it might be worth discussing whether we want to maintain this convention or consider a more descriptive alias.
There was a problem hiding this comment.
I'm pretty sure this was introduced when transitioning form a single-file package called grass(.script?) to multiple files (modules). Functions in original grass or something like that were moved core and grass or grass.script got new functions. grass.script.core then became what grass once was. These imports were the least intrusive change, so it was implemented that way. This was in the pre-v7-times on Subversion trunk, but it was propagated sufficiently through the code base at the time of v7 release that it became a standard for v7. We came up with grass.script as gs as a better practice, but didn't go back (yet) to replace all occurrences of core as grass.
gui/wxpython/startup/locdownload.py
Outdated
| from gui_core.wrap import Button, StaticText | ||
|
|
||
| set_gui_path() |
There was a problem hiding this comment.
Has there been a careful analysis of what is needed for gui_core.wrap import to be successful, and what does set_gui_path provide?
scripts/r.in.wms/wms_drv.py
Outdated
| @@ -18,8 +18,16 @@ | |||
|
|
|||
| import socket | |||
| import grass.script as grass | |||
| import numpy as Numeric | |||
There was a problem hiding this comment.
I know it was there before, but probably a new PR for this, but it is almost an universal convention across all Python code that numpy is imported as np, like import numpy as np. Why should we be different here?
Probably someone from this repo can jump in.
There was a problem hiding this comment.
I think it is just some very old practice carried over. Just change that.
|
@arohanajit Could you take a look at what's left here too, and what should be kept? |
@echoix same here, I am not sure how I can modify an existing PR. So I can make a new commit with changes here which are not incorporated in the current code base |
|
You could add review comments to say what is still relevant or what should be reverted too if you'd want it to be in the same PR. People who can merge (maintainers), can edit the PRs |
echoix
added
the
conflicts/needs rebase
Since these changes are not merged I assume the |
|
Sounds like this can be closed then as already fixed in other PRs. |
Initiated the resolution of Flake8 E402 warnings throughout the codebase by moving imports to the top of the files. However, encountered a scenario where certain module imports are conditional based on the platform (Windows or non-Windows).
Due to platform-dependent imports, moving them to the top would break the conditional logic. To maintain code integrity and adhere to PEP 8, introduced a pattern where platform-specific imports are encapsulated within functions, ensuring they are called when needed.
Following this adjustment, successfully resolved E402 warnings associated with conditional imports.
Subsequently, addressed E302 and E305 warnings by adding appropriate blank lines after import statements and before class/function definitions. This conforms to PEP 8 conventions for improved code readability.
The codebase now aligns with coding standards, maintains platform-specific imports efficiently, and has an enhanced overall structure.
Related to: issue #1522